Skip to content

split up remove_function_pointerst::remove_function_pointer #2870

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 3, 2018

Conversation

kroening
Copy link
Member

This splits up the largest method in the class into two pieces;
there is a natural cut point, which is then also exposed as a public
interface.

Copy link
Contributor

@danpoe danpoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

const code_function_callt &code = to_code_function_call(target->code);

const exprt &function = code.function();
const exprt &pointer = function.op0();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could cast to dereference_exprt and then use .pointer() to make explicit that we're expecting a dereference expression here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

void remove_function_pointer(
goto_programt &goto_program,
goto_programt::targett target,
const functionst &functions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be protected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to expose this, for alternative ways of determining those functions.

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely a good idea, but now we have two functions with the same name, and neither of which are documented. I don't think RTFS makes for good-enough documentation in this case. Please add a brief documentation header to the new one at least (as it is also publicly exposed), or even better: to both of them.

Copy link
Collaborator

@tautschnig tautschnig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use Doxygen-style formatting for the comment, otherwise ok.

@kroening
Copy link
Member Author

kroening commented Sep 3, 2018

Done

const code_function_callt &code = to_code_function_call(target->code);

const exprt &function = code.function();
PRECONDITION(function.id()==ID_dereference);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-format

Daniel Kroening added 2 commits September 3, 2018 21:16
This splits up the largest method in the class into two pieces;
there is a natural cut put, which is then also exposed as a public
interface.
@tautschnig tautschnig force-pushed the split_remove_function_pointer branch from ef87ddd to 10d8b2f Compare September 3, 2018 21:17
@tautschnig tautschnig merged commit c269ca5 into develop Sep 3, 2018
@tautschnig tautschnig deleted the split_remove_function_pointer branch September 3, 2018 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants